[core] Pre filter sources when querying by layer #6514

tmpsantos · 2016-09-29T13:08:56Z

Feature querying is extremelly slow because it goes through all the sources and these sources can be dense. This PR makes it possible to filter by source.

Annotations will benefit from it because they live in their own "virtual" source. Also if you have a low density source for say, road disruptions, you can make the query extremely responsive by querying only on this source.

mention-bot · 2016-09-29T13:08:58Z

@tmpsantos, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @1ec5 and @incanus to be potential reviewers.

tmpsantos · 2016-09-29T13:09:12Z

/cc @brunoabinader

brunoabinader · 2016-09-29T13:25:30Z

src/mbgl/style/source_impl.cpp

@@ -199,6 +199,10 @@ std::unordered_map<std::string, std::vector<Feature>> Source::Impl::queryRendere
 return result;
 }

+ if (parameters.sourceIDs && parameters.sourceIDs->find(id) == parameters.sourceIDs->end()) {


I'd prefer this check to be done in Style::queryRenderedFeatures - this way we can avoid the creation of result for sources not present in sourceIDs.

tmpsantos · 2016-09-29T14:14:35Z

/cc @zugaldia @incanus think this was affecting Android and iOS at some point, right?

zugaldia · 2016-09-29T14:37:20Z

@tmpsantos This is great. cc: @ivovandongen for adapting the Android API.

tmpsantos · 2016-09-29T14:40:49Z

For the record Annotations will get a perf bump for free.

jfirebaugh

Rather than adding new API surface area, can we optimize this case similarly to mapbox/mapbox-gl-js#2877?

tmpsantos · 2016-09-29T19:20:51Z

Rather than adding new API surface area, can we optimize this case similarly to mapbox/mapbox-gl-js#2877

Not sure about pre-filtering automatically. Seems like a valid optimization when we don't set the source filter, but I like the filter parameter specially for annotations and the use case that motivated me write this PR in the first place: query features on from a source I created using the dataset editor. I wanted these features to get in sync with native views, so they needed to be queried extremely fast.

Looking at the code, the layers are stored in a std::vector and we would have to move it to a std::set to get reasonable performance on pre-filtering. Is there a reason for having it in a std::vector in the first place? Can we do both? i.e pre-filter if filter is not explicit.

1ec5 · 2016-09-29T20:22:12Z

query features on from a source I created using the dataset editor

Wouldn’t #5792 be a more direct API for this use case?

tmpsantos · 2016-09-30T11:05:30Z

Wouldn’t #5792 be a more direct API for this use case?

The low level core API I'm proposing could be used by the platform bindings for implementing this API.

ivovandongen · 2016-09-30T13:35:34Z

Rather than adding new API surface area, can we optimize this case similarly to mapbox/mapbox-gl-js#2877

Isn't this complimentary to explicitly specifying the sources to query? I wouldn't mind a little more api surface to have that flexibility here.

1ec5 · 2016-09-30T15:56:08Z

Wouldn’t #5792 be a more direct API for this use case?

The low level core API I'm proposing could be used by the platform bindings for implementing this API.

This would be a departure from how GL JS implements feature querying: querySourceFeatures() has quite a few nuances that make it good for the annotation querying use case but not a great candidate for folding into queryRenderedFeatures().

Probably lost on the cmake migration

tmpsantos · 2016-10-04T17:41:10Z

@jfirebaugh I re-enabled the benchmarks tests on this PR and wrote a benchmark for querying features. Pre-filtering sources gives a 64x performance bump when the layer you are looking for is in a source not so dense (like annotations or a geojson source).

Explicitly specifying the source makes no measurable difference compared to the pre-filter approach and exposes a new API surface. So let's do how you suggested. 👍

tmpsantos · 2016-10-04T17:42:22Z

Pre-filtering sources:

Benchmark                              Time(ns)    CPU(ns) Iterations
---------------------------------------------------------------------
API_queryRenderedFeatures              14167512   14160000         50                                 
API_queryRenderedFeaturesLayerFilter      21569      21563      33019

Before the patch:

Benchmark                              Time(ns)    CPU(ns) Iterations
---------------------------------------------------------------------
API_queryRenderedFeatures              14428871   14400000         50                                 
API_queryRenderedFeaturesLayerFilter    1309461    1304854        515

jfirebaugh

Nice!

brunoabinader reviewed Sep 29, 2016

View reviewed changes

tmpsantos force-pushed the tmpsantos-query_source_filter branch from 0ecabc0 to d0ceb18 Compare September 29, 2016 13:28

tmpsantos force-pushed the tmpsantos-query_source_filter branch from d0ceb18 to 266ad02 Compare September 29, 2016 14:26

ivovandongen mentioned this pull request Sep 29, 2016

[android] query rendered features by source #6516

Closed

tmpsantos force-pushed the tmpsantos-query_source_filter branch from 266ad02 to 34dc6de Compare September 29, 2016 15:11

jfirebaugh suggested changes Sep 29, 2016

View reviewed changes

1ec5 mentioned this pull request Sep 29, 2016

Query visible features in a single MGLStyleLayer #6520

Closed

tmpsantos force-pushed the tmpsantos-query_source_filter branch from 34dc6de to b178987 Compare October 4, 2016 11:56

tmpsantos added 3 commits October 4, 2016 20:33

[core] Pre-filter sources based on the layers they contain

b30beef

[build] Re-add benchmark tests to the buildsystem

8932cb2

Probably lost on the cmake migration

[tests] Add unit tests for query features fiters

85854b4

tmpsantos force-pushed the tmpsantos-query_source_filter branch from b178987 to 5e15919 Compare October 4, 2016 17:35

tmpsantos changed the title ~~[core] allow filter queries by source~~ [core] Pre filter sources when querying by layer Oct 4, 2016

jfirebaugh approved these changes Oct 4, 2016

View reviewed changes

[tests] Added benchmarks for querying for features

bd21249

tmpsantos force-pushed the tmpsantos-query_source_filter branch from 5e15919 to bd21249 Compare October 4, 2016 21:11

tmpsantos merged commit a38104e into master Oct 5, 2016

tmpsantos deleted the tmpsantos-query_source_filter branch October 5, 2016 08:12

1ec5 mentioned this pull request Oct 17, 2016

Update changelogs #6714

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Pre filter sources when querying by layer #6514

[core] Pre filter sources when querying by layer #6514

tmpsantos commented Sep 29, 2016 •

edited

Loading

mention-bot commented Sep 29, 2016

tmpsantos commented Sep 29, 2016

brunoabinader Sep 29, 2016

tmpsantos commented Sep 29, 2016

zugaldia commented Sep 29, 2016

tmpsantos commented Sep 29, 2016

jfirebaugh left a comment

tmpsantos commented Sep 29, 2016 •

edited

Loading

1ec5 commented Sep 29, 2016

tmpsantos commented Sep 30, 2016

ivovandongen commented Sep 30, 2016

1ec5 commented Sep 30, 2016

tmpsantos commented Oct 4, 2016 •

edited

Loading

tmpsantos commented Oct 4, 2016 •

edited

Loading

jfirebaugh left a comment

[core] Pre filter sources when querying by layer #6514

[core] Pre filter sources when querying by layer #6514

Conversation

tmpsantos commented Sep 29, 2016 • edited Loading

mention-bot commented Sep 29, 2016

tmpsantos commented Sep 29, 2016

brunoabinader Sep 29, 2016

Choose a reason for hiding this comment

tmpsantos commented Sep 29, 2016

zugaldia commented Sep 29, 2016

tmpsantos commented Sep 29, 2016

jfirebaugh left a comment

Choose a reason for hiding this comment

tmpsantos commented Sep 29, 2016 • edited Loading

1ec5 commented Sep 29, 2016

tmpsantos commented Sep 30, 2016

ivovandongen commented Sep 30, 2016

1ec5 commented Sep 30, 2016

tmpsantos commented Oct 4, 2016 • edited Loading

tmpsantos commented Oct 4, 2016 • edited Loading

jfirebaugh left a comment

Choose a reason for hiding this comment

tmpsantos commented Sep 29, 2016 •

edited

Loading

tmpsantos commented Sep 29, 2016 •

edited

Loading

tmpsantos commented Oct 4, 2016 •

edited

Loading

tmpsantos commented Oct 4, 2016 •

edited

Loading